docs(spec): ApproverType.describe() qualifies manager instead of offering it as a bare allowed value - #17640
Conversation
…fering it bare `ApproverType` carried no `.describe()` at all, so the generated reference page `content/docs/references/automation/approval.mdx` rendered `## ApproverType` with nothing but an `### Allowed Values` list: `manager` — the one rung an author cannot operate on a stock install, because `sys_user.manager_id` has no product write surface — read exactly like the nine members that work. The describe qualifies it and POINTS at the remedy rather than restating it: `MANAGER_ONLY_REMEDY` / `MANAGER_ONLY_ROUTES` in `packages/lint/src/validate-approval-approvers.ts` stay the single authoritative copy, and that file's `DEPENDENCY` docblock now names this new string among the lines that go stale if the column ever gains a write surface. No enum member is added, removed or renamed. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MkQhmuuJAVDjmeWNixwDDH
`pnpm --filter @objectstack/spec gen:docs` rewrote all 222 reference pages; exactly one changed. The `## ApproverType` section now carries the qualifying describe above its `### Allowed Values` list, and the `type` rows of `ApprovalNodeApprover` and `ApprovalNodeConfig.approvers` — previously blank — carry it too. The page was never hand-edited. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MkQhmuuJAVDjmeWNixwDDH
📓 Docs Drift CheckThis PR changes 2 package(s): 1 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
⛔ 1 release-owned page(s) also name something this change touched. These are read-only:
What this run could not see
Coarse fallback — 135 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 10ff2592b80ac969d9eaf7b0c968c1533c0a5923 && git checkout 10ff2592b80ac969d9eaf7b0c968c1533c0a5923
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 76c9fab30ca406b7b1f06b8ca3db286af9f2bf8b 1aca42a2d7599ada6b901c9a56a85f7293b8d757 && git checkout -B drift-repro 76c9fab30ca406b7b1f06b8ca3db286af9f2bf8b && git merge --no-ff 1aca42a2d7599ada6b901c9a56a85f7293b8d757
node scripts/docs-audit/affected-docs.mjs --json 76c9fab30ca406b7b1f06b8ca3db286af9f2bf8b
|
…ce, and the `manager` rung stops resolving to nobody (objectstack-ai#17993) Fixes objectstack-ai#16678 Clause-②: no Phase 3 of the manager relationship, executed against the director-seat ruling (comment `5651634638`, decision batch objectstack-ai#127 item 1, option B) on the Phase 2 design (comment `5648180780`). Maintainer, verbatim — kept in the original, because rewriting the quotation rewrites the ruling: > 同意 经理 = 管理员在用户上显式设置的 `manager_id`;部门负责人 = 单元上的 `manager_user_id`,两者独立。 ## The hole, reproduced before it was closed `sys_user.manager_id` drives the approvals `{ type: 'manager' }` rung and the `own_and_reports` read scope, and **no product surface could write it**. Measured on the branch point: | route | reading | control | | :--- | ---: | :--- | | `admin-import-users.ts` matches `manager_id` | **0** | `phone_number` **8**, same file, same scan | | whole of `plugin-auth/src` writing `manager_id` | **0** | 11 total matches, every one a test assertion, a comment, or `managed-extension-fields.ts` listing it as *not* editable | | ADR-0092 D2 managed-update whitelist for `sys_user` | `{name, image, locale}` | — | So on any install without a directory sync the rung expanded to nobody. ## What this delivers **1. The endpoint.** `POST /api/v1/auth/admin/set-user-manager`, body `{ userId, managerId }`, `managerId: null` clears. An ObjectStack mount on the raw app ahead of the better-auth catch-all — the `unlock-user` / `import-users` family — platform-admin gated (ADR-0068), ledgered in `auth-route-ledger.ts` and added to the conformance pin that asserts the `source: 'objectstack'` set exactly. **2. It reaches the column by CONTEXT, never by whitelist.** The handler runs under a system context, exactly as `admin-import-users` already reaches `phone_number` and `role`; both write guards gate on `isUserContextWrite`, spelled `Boolean(userId) && isSystem !== true`. So `SYS_USER_PROFILE_EDIT_FIELDS`, `MANAGED_EXTENSION_EDITABLE_FIELDS.sys_user` and `manager_id`'s `readonly: true` are all **unmoved**, and ADR-0092 D4 holds by construction. A test asserts the whitelist is still exactly `{name, image, locale}`, so a future change that "fixes" this by widening Tier 1 goes red here rather than quietly. **3. Five refusals, every one enforced at the write**, each with its reason in the code: | refusal | `error.code` | status | `error.details.reason` | | :--- | :--- | ---: | :--- | | body shape | `INVALID_REQUEST` | 400 | `invalid_body` | | target user missing | `RESOURCE_NOT_FOUND` | 404 | `user_not_found` | | proposed manager missing | `INVALID_REFERENCE` | 400 | `manager_not_found` | | self-assignment | `INVALID_FIELD` | 400 | `self_assignment` | | cycle | `RESOURCE_CONFLICT` | 409 | `cycle` | | depth cap | `VALUE_OUT_OF_RANGE` | 400 | `max_depth_exceeded` | | cross-organization | `INVALID_REFERENCE` | 400 | `cross_organization` | | directory-owned identity | `PERMISSION_DENIED` | 403 | `idp_provisioned` | The cycle check has to be here: the only manager-chain walkers in the open tree are single-hop (`ApprovalService.lookupManager`, `TeamGraphService.managerOf`, one row each) and the multi-hop resolver ships outside this repo, so nothing downstream catches a loop this endpoint lets in. The walk carries its own `seen` set, so a loop that already exists upstream is reported rather than hung on. The cross-organization refusal is the write-time half of `managerIsProvablyOutsideOrg`, and deliberately shares its "provably" shape: `sys_user` carries no `organization_id`, so `sys_member` rows are the only tenancy fact, and an *absent* fact is not a negative one. Both halves are wanted — data drifts after a write. The `idp_provisioned` refusal is the ruling's row 5 (P1, directory wins per identity), keyed on the `sys_user.source` column the platform already stamps. It applies to the clear as well as the set, because both are writes the next sync would revert — the shape ADR-0049 exists to refuse. **Why these codes and not dedicated ones:** `error.code` is a closed vocabulary (`StandardErrorCode` union `ERROR_CODE_LEDGER`) and both live in `packages/spec`, which this lane is fenced out of. So every code is one this package may already emit, and `error.details.reason` carries the machine-readable discrimination, pinned per refusal by the tests. A dedicated code per refusal is the better shape and is reported as a follow-up rather than reached over the fence for. **4. The lint rule keeps its finding; only its remedy changed.** `approval-approvers-may-resolve-empty` and `stackWiresManagerChain` are untouched as a rule — the dead end it reports **survives** the write surface, because a static check still cannot read the column; only its *cause* became recoverable. `MANAGER_ONLY_REMEDY` / `MANAGER_ONLY_ROUTES` now name the endpoint, its body, how to clear the link and what it refuses; the docblock's graded routes are re-taken, and the two "NOT here" grades (SCIM, bulk import) re-measured unchanged. **5. The docs callout** in `content/docs/automation/approvals.mdx` carries the same rewrite in prose, including the directory-precedence rule. ## STOPPED AT THE `packages/spec` FENCE — exactly one part `ApproverType.describe()` in `packages/spec/src/automation/approval.zod.ts` still asserts "*that column has no product write surface*", and that sentence stops being true with this PR. Nothing is pushed for it here. Verified on `origin/main` rather than assumed, as the dispatch required: commit `2eb472418` (objectstack-ai#17640) **is** an ancestor of HEAD — `git merge-base --is-ancestor` exit **0**, which is self-certifying and needs no control leg. But what that commit landed is a `.describe()` that *points* at the remedy while still carrying the now-false assertion. So the third dependent is landed and stale, not landed and fixed. The generated `content/docs/references/automation/approval.mdx` renders the string verbatim and is auto-generated, so it is downstream of the same fenced edit. ⇒ For the `domain:spec` seat. It is the only part stopped at the fence. ## Acceptance notes - **Ruling row 7 (bulk import)** — admitting `manager_id` to the import tier is ruled but is not in this lane's scope fence. `admin-import-users.ts` is untouched and its 0-match reading is re-stated in the lint docblock as still current. - **Ruling row 4 (org-unit derivation)** is cut by the ruling. `sys_business_unit.manager_user_id` and `sys_user.manager_id` stay independent; `BusinessUnitGraphService.headOf` is not consumed. - **The delegated-admin axis** (design §3.3) is not declared. ADR-0068 platform-admin is the floor; a fourth axis on `delegated-admin-gate.ts` is a governed permission-model change and ADR-0049 forbids declaring what is not enforced. Recorded as a follow-up, with the Entra precedent attached in the design. - **The Console affordance** is objectui#9361, a different repo. The design fixes its shape: a read-only lookup in the existing `Organization` group plus a row action posting this endpoint, never a field in the standard edit form. - **Design §8.1 observations, noted and not filed** — this card's own material rather than new defects, and the ruling assigns them to whichever change touches the file first: the dead-slot literal reads `manager:undefined` because `value` is omitted for `manager`; and the objectstack-ai#3807 "expanded to nobody" pin table in `approval-service.test.ts` covers `team` / `department` / `position` / `org_membership_level` and omits `manager`, so that behaviour is real by set membership and not by test. Neither file is touched here. - **`content/docs/permissions/tenant-audit-census.mdx`** and its counts ledger moved 222 to 223 write call sites. That is this PR's own new `engine.update`, regenerated with `scripts/tenant-audit-census.mjs --write`, with the seven hand-written prose figures brought along. ## Clause-② — re-derived from the delivered diff, not predicted New exported symbols in the diff, and whether each is reachable from a published entry. `plugin-auth`'s `exports` map has exactly two entries (`.` to `dist/index.*`, `./rate-limit-storage` to `dist/rate-limit-storage.*`); `admin-set-user-manager.ts` is deliberately **not** re-exported from `src/index.ts` (0 matches). Measured on the rebuilt `dist`, with two positive controls that ARE re-exported, so the scan discriminates: | symbol | `dist/index.d.ts` | `dist/rate-limit-storage.d.ts` | | :--- | ---: | ---: | | `runSetUserManager` | 0 | 0 | | `MAX_MANAGER_CHAIN_DEPTH` | 0 | 0 | | `SetUserManagerDeps` | 0 | 0 | | `SetUserManagerEngine` | 0 | 0 | | `SetUserManagerResult` | 0 | 0 | | `SetUserManagerRefusalReason` | 0 | 0 | | `runAdminBanUser` (control) | **2** | 0 | | `SYS_USER_PROFILE_EDIT_FIELDS` (control) | **3** | 0 | No already-published payload gains a key — the endpoint's response is a new payload, not a new field on an old one. A new **route** is wire, and wire compatibility is not the Clause-② floor. ⇒ **no**, changeset `patch`. `scripts/pm/check-widening-tells.mjs --declaration no` exits 0 but reports **8 of 8 files NOT MEASURED** ("no declared surface covers it"), so it is not a reading in either direction here and is not offered as one.⚠️ **Declared conflict, not silently resolved.** The ruling's own state paragraph says "the endpoint carries `Clause-②: yes` and goes through contract review". The PM claim declares `Clause-②: no`, explicitly as a prediction to be re-derived from the delivered diff, and names the exact flip condition — a re-export from `plugin-auth/src/index.ts`, or a new key on a published payload. Neither holds, so both carriers now read `no` and agree. If the seat that owns the vocabulary wants the tier anyway on the strength of the ruling's sentence, that is its call to make, and this note is here so it is made rather than inherited. ## Checks Run on `614981107`, which is this branch merged with `origin/main` via `scripts/pm/os-regen-merge.sh`, deps refreshed and every package rebuilt (72/72 turbo tasks) afterwards. | check | result | | :--- | :--- | | `pnpm --filter @objectstack/plugin-auth test` | **110 files, 2340 tests passed** | | `pnpm --filter @objectstack/lint test` | **103 files, 3780 tests passed** | | `pnpm --filter @objectstack/plugin-auth typecheck` | exit 0 | | `pnpm --filter @objectstack/lint typecheck` | exit 0 | | derived gate families (`scripts/pm/dispatch-gates.mjs --commands`) | **94 derived, 94 run, 0 NOT-MEASURED, 0 UNRUN** — reconciled with `--ran` carrying an exit code per family, so the zero is derived rather than claimed | | `pnpm --filter @objectstack/spec check:generated` | all 15 generated artifacts up to date after the merge | | `pnpm exec eslint --no-inline-config` (narrowed) | 7 files, 0 errors, 0 warnings | Three gate families were red on the first sweep and every one was a real finding of this diff, fixed rather than baselined: `check:doc-authoring` (the ledger `note` carried a tracker id — a runtime string no author can resolve; moved to an adjacent comment), `check:where-matcher` (the new test engine double read a WHERE combinator as a field name; it now refuses loudly), and `check:tenant-audit-census` (the new engine write moved the ratchet). Three more answered **exit 3, PREREQUISITE NOT MET** — `check:skill-examples`, `check:dual-build-cjs-loads`, `check:type-check-debt` — which is a statement about the tree and never a pass; the closure they name was built and all three then exited 0. **The eslint narrowing is a measurement, not a skipped run**, so its three pieces of evidence are stated rather than implied: (i) the population comes from `eslint.config.mjs`'s own `files` globs (`**/*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}`), not from a guess; (ii) the file count is read from `--format json` output, 7 entries; (iii) this repo runs one `eslint.config.mjs` which **never enables type-aware linting for any file** — no `parserOptions.project`, no typed rules, stated in that file's own header with a recorded positive control — so nothing in this diff can move the verdict on a file it does not touch. The repo-wide `pnpm lint` is CI's run. --- _Generated by [Claude Code](https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
Part of #17579
ApproverType's accept set is byte-identical (no member added, removed or renamed),check:api-surfaceis green on the rebuiltdist/*.d.ts, andcheck:authorable-surfaceleft the checked-in artifacts untouched. The only new bytes are prose.What this changes
ApproverTypecarried no.describe()at all, so the generated reference page rendered## ApproverTypewith nothing but an### Allowed Valueslist.manager— the one rung an author cannot operate on a stock install — read exactly like the nine members that work.packages/spec/src/automation/approval.zod.ts— the enum gains a.describe()that qualifiesmanagerand points at the remedy instead of restating it.content/docs/references/automation/approval.mdx— regenerated, never hand-edited (see the generator evidence below).packages/lint/src/validate-approval-approvers.ts— one paragraph added to theDEPENDENCYdocblock aboveMANAGER_ONLY_REMEDY, naming the new describe among the lines that go stale ifmanager_idever gains a write surface. The triage comment called this out as the part a round is most likely to skip; it is a comment only.The describe, verbatim:
Route (iii), the triage default: it points, it does not restate.
MANAGER_ONLY_REMEDY/MANAGER_ONLY_ROUTESstay the single authoritative copy of the 667-character remedy. No third copy was written, so there is nothing new to keep in step — a pointer cannot drift into disagreement with what it points at. ⛔ No export was added topackages/spec(route (ii) was fenced).Prerequisite readings — taken by state on
origin/main@ea2940d1c4, not inherited from the card1. The⚠️ Correction to the card:
managerdescribe.ApproverTypehad no.describe()to read unqualified — it had none. Its JSDoc (approval.zod.ts:24-30) renders nowhere:renderSchemaSection(packages/spec/scripts/lib/schema-section.ts:328-330) printsmainDef.description, which only.describe()/.meta({description})fills. Positive control that the mechanism is real:HttpMethod's.describe()(packages/spec/src/shared/http.zod.ts:45) renders atcontent/docs/references/shared/http.mdx:46. The card's substance stands — the page soldmanagerunqualified — but the fix is a describe added, not a describe edited.2. The generated page.
content/docs/references/automation/approval.mdx:113is## ApproverType,:115is### Allowed Values,:117is themanagerbullet — the exact window the card names. Its banner (:6) still reads⚠️ AUTO-GENERATED — DO NOT EDIT. Run build-docs.ts to regenerate.and the source callout (:9-10) still namespackages/spec/src/automation/approval.zod.ts. Both hold.3. ⭐
sys_user.manager_idhas no product write surface — re-measured, with the negative leg lit.A throwaway probe drove the identity write guard's real
beforeUpdatehandler in a user context, then was deleted (working tree verified clean afterwards); both legs went through the same handler:{ id, name, manager_id }came back as{ id, name }—manager_idnever reaches the row.{ id, manager_id }alone is refused loudly:PERMISSION_DENIED/403.locale— a field that IS writable — passed the same probe unchanged. ⇒ the instrument is not a refusal that refuses everything.identity-write-guard.test.ts:132(/Editable fields: name, image, locale/) and:186(getManagedUpdateWhitelist('sys_user')equalsnew Set(['name','image','locale'])). Both green in the same run.readonly: trueon the column, with a discriminating control —readonlyoccurrences inside each field block ofpackages/platform-objects/src/identity/sys-user.object.ts:readonlyoccurrencesmanager_idprimary_business_unit_idlocale(control — writable)name(control — writable)No accepting route: across the 59 non-test
packages/plugins/plugin-auth/src/*.tsfiles (enumerated withgit ls-tree, not by grepping contents for a filename),manager_idoccurs 5 times against a firing control ofphone_numberat 28 — and all 5 sit inmanaged-extension-fields.tsandsys-user-writable-fields.ts, both of which name the column only to record that it is not writable (MANAGED_EXTENSION_EDITABLE_FIELDS.sys_userisnew Set(['locale'])). Counts aregrep -o | wc -l, notgrep -c.⇒ The premise holds.
premise_still_valid: true.4. What PR #17575 already fixed — not redone. The
managercallout incontent/docs/automation/approvals.mdx:65-87and the qualification incontent/docs/capabilities/approvals.mdx:14-19are both present and untouched by this PR (git diffnames neither file).The page was regenerated, not hand-edited
The generator rewrote all 222 reference pages and exactly one moved. Beyond the
## ApproverTypesection, the describe also reached the previously blank Description cells of thetyperows inApprovalNodeApproverandApprovalNodeConfig.approvers— measured on the regenerated output, not predicted from the zod registry semantics (which read the other way).Verification
pnpm --filter @objectstack/spec check:generated— all 15 green, includingcheck:api-surface,check:authorable-surface,check:docs.node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstackover this diff: 100 families derived, 98 run green, 0 unrun, 2 NOT MEASURED. Reconciled with--rancarrying each recorded exit code.pnpm check:dual-build-cjs-loadsandpnpm check:lean-entry-closureboth exit 3 — PREREQUISITE NOT MET (they load built entry points repo-wide and this worktree has no fullpnpm build). ⛔ Recorded as NOT MEASURED, not as green; CI builds and runs both.check:skill-examplesfirst exited 1 for want ofpackages/client-react/dist, a prerequisite refusal, not a red. Afterpnpm --filter @objectstack/client-react buildit is green —✅ 258 prose examples type-check across 3 surface(s).pnpm --filter … run test --concurrency=2forwarded--concurrencyinto the vitest script and failed on flag parsing. Re-run correctly aspnpm --workspace-concurrency=2 --filter … run test.pnpm --workspace-concurrency=2 --filter @objectstack/spec --filter @objectstack/lint run test— spec 473 files / 13429 tests passed, lint 103 files / 3747 tests passed.pnpm --workspace-concurrency=2 --filter @objectstack/spec --filter @objectstack/lint run typecheck— exit 0, both test-layer debt ledgers held.pnpm lint(eslint . --no-inline-config, the whole repo) — exit 0. No narrowing was needed, so none is claimed.pnpm check:nul-bytesgreen, plus a direct control-character scan over the four touched files (grep -naP '[\x00-\x08\x0b\x0c\x0e-\x1f\x7f]'→ no match).Heavy runs went through
scripts/pm/os-verify-lock.sh; the verdicts above are itsVERDICT command-exitlines, with each exit code captured before any pipe.Changeset
.changeset/17579-approver-type-manager-describe.md—@objectstack/spec:patch. The describe ships:@objectstack/spec'sfiles[]carriesdist,json-schemaandsrc/**/*.zod.ts, and the new string is measured in all three on the built tree —dist/automation/index.js+.mjs(2 files, against a lit control of an existing describe from the same module, also 2), fourjson-schema/documents, and the shippedapproval.zod.tssource. Prose only, no surface widening ⇒patch, notminor.@objectstack/lintis deliberately not graded. It publishesdistonly, and the new docblock sentence is absent from it (0 files) while a runtime string from the same source file is present in 4 and a pre-existing comment from the same docblock is absent in 0 — comments are stripped by construction, so nothing published moves there.Out of scope
managerenum member is not removed. Removing a member is a larger question and needs its own ruling.managerapprover as a directory-sync dependency #17575's callouts, no change toApproverType's shape, no touch toplugin-author the write guards, no export added topackages/spec.content/docs/releases/untouched.{ type: 'manager' }resolves a column no product surface can write:sys_user.manager_idis refused by the data API and absent from the auth admin endpoints #16678 holds the open question of whethermanager_idgains a product write surface. This wording is correct under every one of its options — the column still needs populating beforemanagerresolves — and theDEPENDENCYdocblock now lists this string among the lines that would need updating in the same change that opens such a surface.Generated by Claude Code